NetworkClock: add manually advanced clock for deterministic tests - #74
Conversation
tfpauly
left a comment
tfpauly
left a comment
There was a problem hiding this comment.
Very cool! Mainly just questioning if we can use a better availability check.
| public struct Instant: InstantProtocol, CustomStringConvertible { | ||
| var time: NetworkDuration | ||
|
|
||
| #if !DisableDebugLogging |
There was a problem hiding this comment.
Seems a bit odd to tie this to debug logging — I assume this is just intended to compile stuff out on prod / release builds?
There was a problem hiding this comment.
Yeah pretty much to avoid the memory cost of adding a class. The CPU cost of the extra check in now() isn't that bad.
There was a problem hiding this comment.
Can we tie it being a debug build or some other trait? I don't think it necessarily makes sense to have this be the logging trait.
There was a problem hiding this comment.
We can't tie it to a debug build because we run tests in release mode AFAIK.
I didn't want a trait just for this, that's why I used the debug logging trait which is always enabled during the tests
There was a problem hiding this comment.
I don't think this should be tied to a Trait at all, it should just use a new Define. Traits are used to allow consumers of a package to modify functionality (by enabling a Define in that package). This only impacts our tests, which consumers of a package don't need to care about, so it should just be a local Define.
There was a problem hiding this comment.
!DisableDebugLogging is ON by default, that's why I used it. We can make it a define, but it should be on by default so that we don't have to manually turn something on just to be able to run the tests locally.
There was a problem hiding this comment.
Yeah I think that's fine personally. Toggling it from the default position becomes an optimization.
- Replace configurable-initial-value clock with a manual time backing store that tests can freeze and advance by hand - Route `now` and `nowAbsolute` through manual time when set, falling back to the system clock otherwise - Track continuous and absolute time separately, preserving nanosecond resolution beyond the system clock's microsecond truncation - Add tests covering freezing, advancing, and restoring the clock
|
|
||
| override func setUpWithError() throws { | ||
| #if !NETWORK_INTERNAL_TESTS | ||
| throw ManualClockUnavailable() |
There was a problem hiding this comment.
Will this cause just running swift test to always fail?
There was a problem hiding this comment.
Yes, if you do not compile swift test with NETWORK_INTERNAL_TESTS, it will fail.
4 participants
store that tests can freeze and advance by hand
nowandnowAbsolutethrough manual time when set, fallingback to the system clock otherwise
resolution beyond the system clock's microsecond truncation